Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSH: add reattach key action #553

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

SSH: add reattach key action #553

wants to merge 2 commits into from

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jan 7, 2025

ref https://github.com/readthedocs/readthedocs-corporate/pull/1926
Screenshot 2025-01-07 at 16-58-16 h1 hello _h1 - SSH keys - Read the Docs

Screenshot 2025-01-14 at 13-45-14 h1 hello _h1 - SSH keys - Read the Docs

Screenshot 2025-01-14 at 13-45-42 h1 hello _h1 - SSH keys - Read the Docs

Also, it's a little weird that we are using a listing UI, when we have support for only one key at the moment.

@stsewd stsewd marked this pull request as ready for review January 14, 2025 20:42
@stsewd stsewd requested a review from a team as a code owner January 14, 2025 20:42
@stsewd stsewd requested a review from agjohnson January 14, 2025 20:42
{% block top_menu %}
<div class="ui stackable text menu">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is always for adding objects to a listing of objects, with a few exceptions for export/download buttons. I would not repurpose this to fit this UI or other one-off buttons and would move this UI outside this partial template and put the content in a .ui.segment.

This also fits in the listing as a small button menu for each key, other UIs use this approach and it would fit if we make SSH keys many to many

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be easier to do this the first time if we had documented patterns or guidelines for this stuff. Whenever I edit the templates it feels like I'm just guessing at where to put stuff and what classes to use, and then just waiting to be told I'm doing it wrong. There has to be a better pattern.

Comment on lines +47 to +50
{% blocktrans trimmed %}
Alternatively, you can
<a href="https://docs.readthedocs.io/en/stable/guides/importing-private-repositories.html#copy-your-project-s-public-key">manually add the SSH key to your Git provider</a>.
{% endblocktrans %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not give two separate actions in this UI, I'd drop this suggestion. It can be a help topic if we need but the first UI element shouldn't point users to manually manage this.

</div>

{% if not can_reattach_key_automatically %}
<div class="ui warning message">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like an error state the user needs to fix. We don't want to send the user down the rabbit hole of fixing the connection to a repository unless their builds aren't working though.

The user isn't going to know of what the difference is between reattaching and "can't automatically reattach", this is fairly technical.

I think this content should be reframed a bit. If the user can reattach an SSH key, the message around this button should be "If you are having trouble building, you can reattach the SSH key". This is missing from the explanation right now.

In the case the user can't reattach, I don't think it should be an error about not being able to reattach, but instead "To enable SSH key management, connect a repository". That is, avoid describing this visually and with copy as an error state.

All of this might fit better in a modal really, but either options works for a first pass.

@stsewd
Copy link
Member Author

stsewd commented Jan 15, 2025

@agjohnson so from what I understand:

  • Put the "reattach" button inside the list, even if we only have one item at the moment. (should also be exposed in the detail view?)
  • Show a modal if the action can't be performed
  • Link to the manual steps page as a help topic instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants